Walkthrough

Adds persistent HTTP server mode for ec validate input, with new flags, request handling, lifecycle wiring, acceptance coverage, and updated docs/tests.

Changes

Validate Input Server Mode

Estimated code review effort: 4 (Complex) | ~60 minutes

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Review — Approve

This PR adds a persistent HTTP server mode to ec validate input, enabling policy evaluation over REST via POST /v1/validate/input. The implementation is well-structured, with good separation of concerns across internal/server/, proper signal handling for graceful shutdown, and comprehensive test coverage at both unit and acceptance test levels.

Strengths

• Secure defaults: binds to 127.0.0.1 by default, enforces a 10 MB request body limit, includes a 90-second evaluation timeout, and documents the lack of built-in auth/rate limiting
• Resilient design: recovery middleware catches panics, structured JSON error responses avoid leaking internal paths, and the ready endpoint defers 200 until policy sources are fully loaded
• Correct shutdown: signal.NotifyContext in cmd/root.go propagates SIGINT/SIGTERM through the context, giving the HTTP server a 10-second graceful shutdown window
• Reuse of existing evaluation pipeline: the server reuses output.Output, input.Report, and the evaluator interface, keeping behavior consistent with ec validate input --output json
• Good test coverage: unit tests cover all handler paths (empty body, invalid input, success, violations, timeout, evaluation error, recovery middleware, server lifecycle), plus four BDD acceptance scenarios

Findings

1. Concurrency contract on Evaluator — documented but not stress-tested (low)

The new doc comment on Evaluator states "must be safe for concurrent use," and the server shares evaluator instances across concurrent HTTP requests. Code analysis confirms this is safe: conftestEvaluator.Evaluate has a value receiver (each call gets a copy of the struct), policy downloads are cached via sync.OnceValues, and the conftest runner is created locally per call. However, this is the first code path that actually exercises shared evaluators under concurrency (the non-server worker pool creates separate evaluators per goroutine). Consider adding a Go race-detector test (go test -race) that fires concurrent requests at the handler to prove this empirically.

2. mockEvaluator.target field has no synchronization (low)

In internal/server/server_test.go, the mock stores the evaluation target via m.target = target without synchronization. This is safe in the current sequential tests but would trigger a data race under -race if a concurrent test were added. A minor future-proofing concern.

3. Flag help text says "Mutually exclusive with --file" (low)

The --server flag description says "Mutually exclusive with --file" but the actual PreRunE check covers all input file sources (including positional arguments). The error message correctly says "--server and input files are mutually exclusive." The help text could be updated to say "Mutually exclusive with input files" for precision.

4. Unrelated Makefile change included (low)

The lint/lint-fix targets switch from addlicense ... . to git ls-files -z | xargs -0 addlicense ..., scoping license checks to git-tracked files only. This is a reasonable improvement (avoids checking generated/vendored files) but is orthogonal to the server feature. Consider separating it if this PR needs to be reverted independently.

Architecture Notes

• The choice to load policies once at startup and reuse evaluators is correct for a persistent server — it avoids repeated downloads and OPA compilation. The trade-off (restart to pick up policy changes) is clearly documented.
• Returning HTTP 200 for all completed evaluations (with success: false for violations) follows API best practices — HTTP status codes reflect transport/server errors, not application-level outcomes.
• The input.NewInput(ctx, nil, s.cfg.Policy) call passes nil for paths, which is correct since the server receives inputs per-request rather than at startup.

Labels: PR adds a new HTTP server mode to the CLI, touching CLI flags, internal server package, evaluator interface docs, and acceptance tests

Review — comment

Scope: New --server mode for ec validate input — adds a persistent HTTP/REST server for policy evaluation with health probes, graceful shutdown, and signal handling.

Summary

This is a well-structured feature addition that introduces server mode to the existing validate input command. The architecture is sound: evaluators are initialized once at startup, policy downloads are cached via sync.OnceValues, and each request creates its own conftest runner and temp file, making concurrent request handling safe. The PR includes thorough unit tests (handler, middleware, lifecycle, timeout), acceptance tests (4 Gherkin scenarios), documentation updates, and proper signal handling in cmd/root.go.

Findings

1. Server binds to all network interfaces by default — medium / security

File: internal/server/server.go (line ~89 in new file)
Code: Addr: fmt.Sprintf(":%d", s.cfg.Port)

The server binds to 0.0.0.0 (all interfaces) by default. The existing TODO acknowledges this (// TODO: add a --server-address flag), but shipping without a --server-address flag means the server is network-accessible from any interface out of the box. Users running ec validate input --server on a machine with a public IP will expose the unauthenticated evaluation endpoint externally.

Remediation: Consider defaulting Addr to 127.0.0.1:%d (localhost-only) and adding --server-address to allow explicit opt-in to network exposure. This follows the principle of least privilege — local-only by default, explicitly bindable to 0.0.0.0 when needed. If deferring this to a follow-up, the documentation and --server help text should warn that the server listens on all interfaces.

2. No authentication or rate limiting on evaluation endpoint — medium / security

File: internal/server/server.go, internal/server/handler.go

The server exposes POST /v1/validate/input with no authentication, authorization, or rate limiting. Each evaluation request triggers policy evaluation with a 90-second timeout, and there is no limit on concurrent evaluations. An adversary (or misconfigured client) could exhaust server resources by sending many concurrent requests.

This is acceptable for an initial implementation, but should be documented as a known limitation. The combination of network-accessible binding (finding #1) and no auth means anyone who can reach the port can consume server resources.

Remediation: For the initial PR, at minimum add a note to the command help text and docs that the server has no built-in authentication and should be deployed behind a reverse proxy or network policy in production. Consider adding --max-concurrent-evaluations as a follow-up to bound resource usage.

3. Concurrent request safety relies on evaluator implementation details — low / correctness

File: internal/server/server.go (lines ~70-80 in new file), internal/server/handler.go

The server comment correctly notes that evaluators are safe to share because policy downloads are cached via sync.OnceValues and each Evaluate call creates its own conftest runner. I verified this: conftestEvaluator.Evaluate (conftest_evaluator.go:414) creates a fresh conftestRunner per call, and the shared fields (workDir, policyDir, dataDir) are read-only after init. The downloadCache uses sync.Map.

However, this safety guarantee is implicit — there's no interface contract or documentation in the Evaluator interface stating that Evaluate must be safe for concurrent use. If a future evaluator implementation is added that mutates shared state in Evaluate, the server would silently break.

Remediation: Consider adding a // Evaluate is safe for concurrent use comment to the Evaluator interface definition, or document this requirement in the interface's godoc.

Positive observations

• Graceful shutdown is properly implemented with signal.NotifyContext in cmd/root.go and a 10-second shutdown timeout in the server.
• Request body validation enforces a 10MB limit, checks for empty bodies, and validates JSON/YAML format before processing.
• Error information leakage prevention — internal error details (file paths, engine errors) are not exposed to API clients.
• Recovery middleware prevents panics from crashing the server.
• Test coverage is strong: unit tests cover all handler paths (empty body, invalid input, success, violations, evaluation error, timeout, YAML input), middleware, and server lifecycle. Acceptance tests cover end-to-end scenarios.
• Proper temp file cleanup via defer os.Remove(tmpPath) prevents disk exhaustion.
• Status logging middleware with latency tracking aids operational monitoring.

Verdict

The implementation is solid and well-tested. The two medium findings are security hardening items that should be tracked for follow-up before production deployment, but don't block the feature from landing. The requires-manual-review label is appropriate given the scope of this change.

Note: This PR carries the requires-manual-review label. Automated review covers code-level concerns; human reviewers should additionally assess the operational deployment model and whether the security posture is acceptable for the intended use case.

Labels: PR adds new server/HTTP feature with security surface and CLI integration

Review — ec validate input --server

This PR adds a persistent HTTP/REST server mode to ec validate input, exposing policy evaluation via POST /v1/validate/input with Kubernetes-style health probes (/live, /ready). The implementation is well-structured: clean separation of concerns across server.go, handler.go, and middleware.go; proper signal handling in cmd/root.go; comprehensive unit and acceptance tests.

Findings

1. Concurrent evaluator safety — shared evaluators across HTTP requests (medium)

File: internal/server/handler.go (line ~97), internal/server/server.go (line ~74)

The server loads evaluators once at startup (input.NewInput) and reuses them across all concurrent HTTP requests:

for _, e := range s.evaluators {
    results, err := e.Evaluate(evalCtx, evaluator.EvaluationTarget{Inputs: []string{tmpPath}})
}

The conftestEvaluator.Evaluate() method re-downloads policy sources into its workDir on every call (s.GetPolicy(ctx, c.workDir, false)), creates a test runner referencing shared directories (c.policyDir, c.dataDir), and performs file I/O against those paths. When multiple HTTP requests invoke Evaluate concurrently on the same evaluator instance, these unsynchronized file system operations can race.

The CLI avoids this because each worker in validateInputCmd.RunE creates its own evaluator via ValidateInput → input.NewInput. The server's architecture differs — evaluators are shared.

Remediation: Either (a) create per-request evaluator instances (matching the CLI's per-worker pattern), (b) serialize evaluation with a mutex/semaphore, or (c) verify and document that GetPolicy is idempotent/safe for concurrent reads after initial download. Option (b) with a bounded semaphore would also provide natural backpressure.

2. Server binds to all interfaces by default (low)

File: internal/server/server.go (line ~87)

Addr: fmt.Sprintf(":%d", s.cfg.Port),

The server binds to 0.0.0.0, making it network-accessible on all interfaces. While this is appropriate for containerized/Kubernetes deployments, there's no --server-address flag to restrict binding to localhost for local development or security-sensitive environments.

The documentation curl example shows http://localhost:8080/..., which may give users the impression the server is localhost-only.

Remediation: Consider adding a --server-address flag (defaulting to 0.0.0.0 or "") so users can bind to 127.0.0.1 when desired. This could be a follow-up.

3. Global logger mutation in server mode (low)

File: internal/server/server.go (lines ~59–63)

if log.GetLevel() < log.InfoLevel {
    log.SetLevel(log.InfoLevel)
}
log.SetFormatter(&log.JSONFormatter{})

This modifies the global logrus logger (level and formatter). Since server mode is a terminal execution path (the server runs until shutdown), this is acceptable in practice. The code comment explains the rationale clearly. Noting it as a design choice rather than an issue — if the server were ever embedded in a larger process, this would need revisiting.

4. Port allocation TOCTOU in acceptance tests (low)

File: acceptance/cli/server.go (lines ~91–96)

The test allocates a free port by binding to :0, closing the listener, then starting the server on that port. Between close and bind, another process could claim the port. This is a standard pattern in test code and is mitigated by waitForReady's retry loop, so it's unlikely to cause flaky tests in practice.

Signal handling change scope

The cmd/root.go change replaces context.Background() with signal.NotifyContext for SIGINT/SIGTERM. This affects all commands, not just validate input --server. This is a positive change — it enables graceful cancellation for any command. However, any command that previously assumed the root context would never be cancelled should be verified. Given that Go's standard library and this codebase generally handle context cancellation correctly, the risk is low.

What's done well

• Request body validation — size limits (maxRequestBodySize), content-type-aware JSON/YAML validation, proper error responses
• Error message hygiene — internal details (file paths, engine errors) are not leaked to API clients
• Recovery middleware — panics in handlers don't crash the server
• Graceful shutdown — proper httpServer.Shutdown with timeout, evaluator cleanup via destroyEvaluators
• Test coverage — unit tests for all handlers/edge cases, acceptance tests covering happy path, violations, health endpoints, and invalid input
• Documentation — command help, asciidoc, and examples all updated consistently
• ReadHeaderTimeout set — avoids Slowloris-style attacks (good security practice for Go HTTP servers)

Verdict

The concurrent evaluator safety issue (finding 1) deserves attention before this sees production traffic. Under concurrent load, the shared evaluators could produce incorrect results or crash due to file system races. The remaining findings are low severity and could be addressed as follow-ups.

Action: comment — the concurrent access concern is worth resolving but does not necessarily block merge if the expected initial usage is single-client or low-concurrency.

Labels: PR adds a new HTTP server mode for policy evaluation (new feature), modifies CLI command structure